fix: correct field error labels in purchase confirm action#234
fix: correct field error labels in purchase confirm action#234wheeljackz wants to merge 1 commit intomainfrom
Conversation
The validateRequired() calls had wrong label strings — each field was
labelled with the PREVIOUS field's name, causing confusing error messages
like 'Nomor WhatsApp wajib diisi' when the Bank Name field was empty.
Before:
bankName: validateRequired('Nomor WhatsApp', bankName)
bankAccountNumber: validateRequired('Nama Bank', bankAccountNumber)
bankAccountName: validateRequired('Nomor Rekening', bankAccountName)
After (matching the form field labels):
bankName: validateRequired('Nama Bank', bankName)
bankAccountNumber: validateRequired('Nomor Rekening', bankAccountNumber)
bankAccountName: validateRequired('Nama Pemilik Rekening', bankAccountName)
All 116 tests pass.
wheeljackz
left a comment
wheeljackz
left a comment
There was a problem hiding this comment.
Reviewed all changed files in this PR (1 file): app/routes/dashboard.purchase.confirm.tsx.
Findings:
- Validation label mappings now correctly align with their corresponding form fields (
Nama Bank,Nomor Rekening,Nama Pemilik Rekening,Nominal). - No behavioral regression observed from this change; the action still validates required fields and returns consistent
fieldErrors/fieldsshape. - No new security exposure introduced by this diff (string-only labels passed to existing validator function).
- Type safety remains intact for the modified lines.
- Test suite and type-check pass locally (
npm test,npm run type-check).
VERDICT: NO ISSUES FOUND
wheeljackz
left a comment
wheeljackz
left a comment
There was a problem hiding this comment.
Reviewed the label mapping change. The three replacements are internally consistent with the form labels, but I left one inline note about the remaining server-side error path and its missing regression coverage.
| bankName: validateRequired('Nomor WhatsApp', bankName), | ||
| bankAccountNumber: validateRequired('Nama Bank', bankAccountNumber), | ||
| bankAccountName: validateRequired('Nomor Rekening', bankAccountName), | ||
| bankName: validateRequired('Nama Bank', bankName), |
There was a problem hiding this comment.
These labels now match the <Field label> props below, but this route still never reads useActionData() or passes fieldErrors back into the form. So a failed POST still will not render these server-side labels anywhere, and there is no focused regression test covering that path. If the intent is a user-visible validation fix, we probably need to plumb fieldErrors into the fields or add an action-level test that proves this branch matters.
wheeljackz
left a comment
wheeljackz
left a comment
There was a problem hiding this comment.
Thanks for the cleanup here. I found two follow-ups worth addressing in the diff.
| bankName: validateRequired('Nomor WhatsApp', bankName), | ||
| bankAccountNumber: validateRequired('Nama Bank', bankAccountNumber), | ||
| bankAccountName: validateRequired('Nomor Rekening', bankAccountName), | ||
| bankName: validateRequired('Nama Bank', bankName), |
There was a problem hiding this comment.
These labels are now correct, but action() still only returns fieldErrors; PurchaseConfirm never reads them with useActionData or passes them into the <Field error={...}> props below. That means this fixes the action payload, but not the user-visible validation path in the form itself. If the reported bug was about the message shown after a failed submit, we still need to wire the action data into the fields.
| bankAccountNumber: validateRequired('Nama Bank', bankAccountNumber), | ||
| bankAccountName: validateRequired('Nomor Rekening', bankAccountName), | ||
| bankName: validateRequired('Nama Bank', bankName), | ||
| bankAccountNumber: validateRequired('Nomor Rekening', bankAccountNumber), |
There was a problem hiding this comment.
Can we add a regression test around this mapping? Right now there doesn’t seem to be any route/action coverage for dashboard.purchase.confirm, so these duplicated label strings can drift away from the <Field label=...> props again without anything failing. A small action test that posts empty values and asserts the exact fieldErrors messages would lock this fix in.
1 participant
Summary
The
validateRequired()calls in thedashboard.purchase.confirmaction had wrong label strings — each field was labelled with the previous field's name. This caused confusing validation error messages like"Nomor WhatsApp wajib diisi"when the Bank Name field was left empty.Before (wrong labels)
After (matching form field labels)
Labels now match the corresponding
<Field label="..."\>props in the form component.Testing